Skip to content

Fix module recursive lookup bug #1130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix module recursive lookup bug #1130

wants to merge 2 commits into from

Conversation

@tompng
Copy link
Member

@tompng tompng commented Jun 30, 2024

Fixes this bug

module Mod1
  module Sub
  end
end

module Mod2
  include Mod1
end

module Mod3
  include Mod2
  include Sub
  # 'Mod1::Sub' is included, but RDoc thinks unknown constant 'Sub' is included
end

What was wrong with constant lookup implementation

module M1
  module M2
    include A
    include B

    # Constant lookup of C
    # should search under M2, under M1, recursively B, recursively A, recursively Object
    # Was searching under M2, under B, under A, under M1
    include C

    include D
  end
end

Ruby's actual constant priority
document https://docs.ruby-lang.org/ja/latest/doc/spec=2fvariables.html#prio (Sorry, can't find the english version)

# To check constant lookup priority, comment out C=1, C=2, C=3.0, C=3.1, ... in that order
module A; C=4.0; module AA; C=4.1; end; include AA; end
module B; C=3.0; module BB; C=3.1; end; include BB; end
module X; C=:never; end
C=5.0; module O1; C=6.1; end; include O1 # TopLevel ≒ Object
class Object; C=5.1; module O2; C=6.0; end; include O2; end
module M1
  include X
  C=2
  module M2
    C=1
    include A
    include B
    p C
  end
end

tompng
tompng commented Jun 30, 2024
View reviewed changes
test/rdoc/test_rdoc_include.rb

assert_equal [i1_m1, i1_m2, i1_m3, i1_m4, i1_k0_m4], m1_k1.includes
assert_equal [m1_m2_k0_m4, m1_m2_m3_m4, m1_m2_m3, m1_m2, m1, @object,
assert_equal [m1_m2_k0_m4, m1_m2_m4, m1_m3, m1_m2, m1, @object,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test was wrong.
Actual Mod1::Klass1.ancestors is

[Mod1::Klass1, Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod4, Mod1::Mod3, Mod1::Mod2, Mod1, Object, Kernel, BasicObject]
module Mod1
  module Mod3
  end
  module Mod2
    module Mod3
      module Mod4
      end
    end
    module Mod4
    end
    class Klass0
      module Mod4
        # module Mod5
        # end
        module Mod6
        end
      end
      module Mod5; end
      include Mod4
      include Mod5
      include Mod6
      include Mod1
      include Mod2
      include Mod3
    end
  end
  class Klass1
    include Mod1
    include Mod2
    include Mod3
    include Mod4
    include Klass0::Mod4
  end
end
p Mod1::Klass1.ancestors
# Actual
#=> [Mod1::Klass1, Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod4,      Mod1::Mod3,       Mod1::Mod2, Mod1, Object, Kernel,    BasicObject]
# Test was expecting
#=> [(omitted),    Mod1::Mod2::Klass0::Mod4, Mod1::Mod2::Mod3,Mod4, Mod1::Mod2::Mod3, Mod1::Mod2, Mod1, Object, (omitted), BasicObject]

@st0012 st0012 added the bug label Jun 30, 2024
@colby-swandale colby-swandale self-assigned this Jul 3, 2024
@colby-swandale colby-swandale self-requested a review July 3, 2024 13:22
@tompng tompng force-pushed the module_lookup_bugfix branch from 5da426b to 47a40af Compare July 4, 2024 15:52
tompng
tompng commented Jul 21, 2024
View reviewed changes
lib/rdoc/code_object/context.rb
full_name = nesting.child_name(name)
mod = @store.modules_hash[full_name]
return mod if mod
nesting = nesting.parent
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this part is not correct. should use Module.nesting instead of CodeObject#parent chain.

- while nesting
-   ...
-   nesting = nesting.parent
- end
+ unavailable_data.module_nesting.each do
+   ...
+ end

But I think RDoc::Include and RDoc::Extend does not provide enough information.
This pull request and the original implementation uses CodeObject#parent chain instead of module nesting.

Complicated example

class ::A
  class ::B
    class ::A
      class ::B::C
        # Module.nesting is [::B::C, ::A, ::B, ::A]
        include X
      end
    end
  end
end

@st0012 st0012 added this to the v6.8.0 milestone Oct 17, 2024
@st0012 st0012 modified the milestones: v6.8.0, v7.0.0 Nov 12, 2024
@st0012 st0012 removed this from the v7.0.0 milestone Jan 28, 2025
@tompng
Copy link
Member Author

tompng commented Feb 11, 2025

Closing this because I think we need to do #1291 rather this pull request. And this pull request might not be the right way.

@tompng tompng closed this Feb 11, 2025
@tompng tompng deleted the module_lookup_bugfix branch February 11, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colby-swandale colby-swandale Awaiting requested review from colby-swandale

Assignees

@colby-swandale colby-swandale

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tompng @colby-swandale @st0012